[pkg/ottl] Change OTTL contexts to handle paths with context #36820
Conversation
github-actions
bot
requested review from
bogdandrutu,
evan-bradley,
kentquirk and
TylerHelmuth
edmocosta
changed the title
…with-context' into change-contexts-to-support-path-with-context
|
Hi @TylerHelmuth @evan-bradley! I've marked this PR as a draft so I can double check my understanding on the final solution, and discuss/implement a topic before starting fully reviewing it. Should we support fully qualified paths? let's take the The above example would infer the In this case, the inferred context would be the No accepting fully qualified paths would make it impossible to users to enforce certain context without using it in the statement. That said, I think we could accept both scenarios, for example, unqualified but correctly inferred from the statement: And fully qualified, aiming enforcing certain context: WDYT? |
|
Thanks @edmocosta, really excited to see this one.
I think that's a great observation. I'm personally in favor of adding this to allow users the ability to manually specify a context, I think we need to retain the ability for users to set contexts in ways not obvious from the paths alone. However, for this case: Could we (maybe in the future if fully-qualified paths will allow us to get around this) add an option to the context inferrer to use context-specific functions as hints for the context a statement should run in? I think this will offer the best UX and allow users to avoid needing to think about how to force a context. |
|
I agree that it seems like we need the function context to be a part of the equation. I'm glad you thought of this example now bc I don't think we discussed this before. |
Yes, we can definitely make the context inferrer smarter adding the functions hint, it would require extracting the functions usage from the parsed statements, and verifying if the configured context parsers supports it, prioritising them accordingly. Even though it would solve the functions issue, we would still need to deal with enums, which are prone to the exactly same problem. Differently from the functions, the We can solve both issues changing the context inferrer - with the cost of increasing complexity a bit - but I'm wondering if we're missing any other edge-case here? Anyway, I don't think it would be a problem if we are - at least for now - considering users are still able to express their Summarising:
That said, If you folks agree, I'll change the context inferrer as discussed, verifying the configured parsers functions, and trying to parse the enums. |
Ya I like this approach as it avoids users having to write |
|
Hey @TylerHelmuth @evan-bradley, thanks again for the help, I've opened a new PR changing the context inferrer as discussed, and including the extra changes of this PR (to make it smaller). Once we get that PR merged, I'll update this one and mark as ready to review. |
…lector-contrib into change-contexts-to-support-path-with-context # Conflicts: # pkg/ottl/contexts/internal/metric.go # pkg/ottl/contexts/internal/resource.go # pkg/ottl/contexts/internal/scope.go # pkg/ottl/contexts/internal/span.go # pkg/ottl/contexts/ottldatapoint/datapoint_test.go # pkg/ottl/contexts/ottllog/log_test.go # pkg/ottl/contexts/ottlscope/scope_test.go # pkg/ottl/contexts/ottlspan/span_test.go # pkg/ottl/contexts/ottlspanevent/span_events_test.go
#36869) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This PR is part of #29017, and a spin-off from #36820. It changes the existing context inferrer logic to also take into consideration the functions and enums used on the statements. (#36820 (comment)) New logic: - Find all `path`, function names(`editor`, `converter`), and `enumSymbol` on the statements - Pick the highest priority context (same existing logic) based on the `path.Context` values - If the chosen context does not support all used functions and enums, it goes through it's lower contexts (wide scope contexts that does support the chosen context as a path context) testing them and choosing the first one that supports them. - If no context that can handle the functions and enums is found, the inference fail and an empty value is returned. The parser collection was adapted to support the new context inferrer configuration requirements. **Other important changes:** Currently, it's possible to have paths to contexts root objects, for example: `set(attributes["body"], resource)`. Given `resource` has no dot separators on the path, the grammar extracts it into the `path.Fields` slice, letting the `path.Context` value empty. Why? This grammar behaviour is still necessary to keep backward compatibility with paths without context, otherwise it would start requiring contexts for all paths independently of the parser configuration. Previous PRs didn't take this edge case into consideration, and a few places needed to be changed to address it: - Context inferrer (`getContextCandidate`) - Parser `prependContextToStatementPaths` function. - Reusable OTTL contexts (`contexts/internal`) (not part of this PR, it will be fixed by #36820) When/If we reach the point to deprecate paths _without_ context, all those conditions can be removed, and the grammar changed to require and extract the `path` context properly. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue #29017 <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests <!--Describe the documentation added.--> #### Documentation No changes <!--Please delete paragraphs that you did not use before submitting.-->
Description
This PR is part of #29017, and changes all existing OTTL contexts to properly handle
ottl.Pathwithottl.Path.Context. (I'm sorry for the big PR, but splitting it wouldn't make much difference considering all changes are related)OTTL contexts changes:
EnablePathContextNames, so API consumers can enable the support to path with context names, previously added through [pkg/ottl] Change grammar to support expressing statements context via path names #34875.parsePathfunctions were modified to properly handle paths with contexts.ContextName(it might be useful to configure the parser collection, for example, but I'm fine hiding it if folks think it's not necessary)Considering we might deprecate the path without context support in the future, and aiming to avoid copy and paste all the existing tests, I've included some logic to dynamically copy the "without path context" tests, into a "with path context" tests, ensuring all paths are tested with both scenarios. I understand those generated tests make it hard to read/understand, but given the above considerations, I think it may be worth it (please let me know if you think otherwise, so I can change the approach).
Other important changes:
Currently, it's possible to have paths to contexts root objects, for example:set(attributes["body"], resource). Givenresourcehas no dot separators on the path, the grammar extracts it into thepath.Fieldsslice, letting thepath.Contextvalue empty. Why? This grammar behaviour is still necessary to keep backward compatibility with paths without context, otherwise it would start requiring contexts for all paths independently of the parser configuration.Previous PRs didn't take this edge case into consideration, and a few places needed to be changed to address it:Context inferrer (including an extra fix of the default inferrer order)ParserprependContextToStatementPathsfunctionReusable OTTL contexts (contexts/internal), to also handleottl.Pathwith only theContextset as an object root access (in addition to the exitingpath == nilcondition)When/If we reach the point to deprecate paths without context, all those conditions can be removed, and the grammar changed to require and extract the context properly.Link to tracking issue
#29017
Testing
Unit tests
Documentation
No changes